Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set defaults to false for loader definitions #1867

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

RichieAHB
Copy link

This usage of defaults causes issues when using descriptors with protobufjs. Descriptors that default to oneofIndex: 0 cause issues when they aren't actually part of a oneof. Specifically here:

https://github.com/protobufjs/protobuf.js/blob/95b56817ef6fb9bdcb14d956c159da49d0889bff/ext/descriptor/index.js#L218-L219

where it assumes that if there's a oneofIndex then that is a valid index into the oneofDecl array. I could have changed the logic there but this feels more "upstream" and there doesn't seem to be a need for defaults to be true regardless of this issue with protobufjs.

There isn't much value in having defaults set for descriptors

What's more it causes issues when using those descriptors with protobufjs

Descriptors with oneofIndex: 0, cause issues when they aren't part of a oneof
@linux-foundation-easycla
Copy link

CLA Missing ID

@murgatroid99
Copy link
Member

Those options aren't used at all when loading descriptor objects in as package definition objects, so this issue with oneofindex just doesn't seem to be relevant.

@RichieAHB
Copy link
Author

They are used in createMessageDefinition which is used by createDefinition, which is used by createPackageDefinition as a means for of creating the definition object. I’m not sure what you mean?

@murgatroid99
Copy link
Member

The Protobuf.js function you pointed to is Type#fromDescriptor. Nothing in the chain of functions you just mentioned calls that function.

@RichieAHB
Copy link
Author

RichieAHB commented Aug 5, 2021

Ah right, I see what you mean, nothing calls that in that chain. That chain is purely for the creation of the descriptor objects that exhibit this problem. So, when using a descriptor that has been created using that chain of functions (i.e. any of the exported helpers from this file) the protobufjs fromDescriptor can break. I.e when trying to create a message type for a request / response from one of the service descriptors returned by this file. Does that make more sense?

@murgatroid99
Copy link
Member

Are you talking about taking that object output by proto-loader and passing it directly to Protobuf.js yourself? If so, that was never an intended use case of this API.

Those message definition and enum definition objects have a structure specified in this proposal. In particular, the type field has the canonical JSON mapping representation of the descriptor message, which is specified to include default values.

@RichieAHB
Copy link
Author

RichieAHB commented Aug 5, 2021

I actually read the opposite to be true (by default) in the linked Google page regarding the canonical JSON mapping:

If a field has the default value in the protocol buffer, it will be omitted in the JSON-encoded data by default to save space.

I understand that it wasn't intended but as per that proposal it might be nice to support it given that the defaults would allow this (from my reading)? I'm assuming this is why protobufjs fails to handle this case, because it does assume defaults will not be present in the JSON mapping: but that's just speculation!

@murgatroid99
Copy link
Member

I see. I did have that backwards. In that case I think I can accept this change. You'll still need to sign the CLA.

As far as I know, Protobuf.js APIs are not generally concerned with the canonical JSON mapping. I assume it just doesn't account for representing the object with defaults.

@RichieAHB
Copy link
Author

Excellent thanks 👍 I have gone through and signed the CLA, do I need to force push a slight change to this branch for it to retrigger?

@murgatroid99
Copy link
Member

Looking at the CLA bot comment again, it looks like the problem is that the commit is using an email address that is not associated with your GitHub account. I think you can fix it by either adding that email address to your GitHub account, or modifying the commit with the email address that you already use with GitHub.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants